Use native DOMParser and XMLSerializer #26
Conversation
| const newAdBreak = new AdBreak( | ||
| adBreak._attributes.timeOffset, | ||
| adBreak._attributes.breakType, | ||
| adBreak.getAttribute('timeOffset') || '', |
There was a problem hiding this comment.
timeOffset from VMAP 1.0 Schema is required and cannot be ''.
| adBreak._attributes.timeOffset, | ||
| adBreak._attributes.breakType, | ||
| adBreak.getAttribute('timeOffset') || '', | ||
| adBreak.getAttribute('breakType') || '', |
There was a problem hiding this comment.
breakType from VMAP 1.0 Schema is required.
| adSources, | ||
| adBreak._attributes.breakId, | ||
| adBreak._attributes.repeatAfter, | ||
| adBreak.getAttribute('breakId') || '', |
There was a problem hiding this comment.
breakId should be optional, but should not be an empty string.
| minBitrate, | ||
| maxBitrate, | ||
| url || '', | ||
| delivery || '', |
There was a problem hiding this comment.
delivery from VAST 4.x is required and must be either "progressive" or "streaming"
| maxBitrate, | ||
| url || '', | ||
| delivery || '', | ||
| type || '', |
There was a problem hiding this comment.
type from VAST 4.x is required and should not be an empty string
| type || '', | ||
| width ? parseInt(width) : 0, | ||
| height ? parseInt(height) : 0, | ||
| codec || '', |
There was a problem hiding this comment.
codec from VAST 4.x takes values as specified by RFC
4281: http://tools.ietf.org/html/rfc4281
| width ? parseInt(width) : 0, | ||
| height ? parseInt(height) : 0, | ||
| codec || '', | ||
| id || '', |
| const trackingEvents = mapArrayOrElem(trackingNodes, tracking => { | ||
| if (tracking.parentNode?.nodeName.toLowerCase() !== 'trackingevents') return null; | ||
|
|
||
| const event = tracking.getAttribute('event') || ''; |
There was a problem hiding this comment.
event should not be an empty string
| }); | ||
| const inLine = new InLine('', '', impressions, creatives, void 0, void 0, void 0, void 0, void 0, errors, extensions); | ||
| const newAd = new Ad(ad._attributes?.id, ad._attributes?.sequence ? parseInt(ad._attributes.sequence) : void 0, inLine); | ||
| const adId = ad.getAttribute('id') || ''; |
| return newAd; | ||
| }); | ||
| const newVast = new VAST(vastObj._attributes.version, ads); | ||
| const newVast = new VAST(vastRoot.getAttribute('version') || '', ads); |
There was a problem hiding this comment.
version should not be an empty string
|
Thank you for the pull request. I will pick this request over #25 after the review is done since the main use case of OMAP is assumed to run it on a Web browser for now, and the request is to optimize performance in that. |
After #25, I've made a further progress in the direction of removing
xml-js, migrating from a xml library that provides their original interface, to Web standard DOMParser and XMLSerializer.With this change the package size with required dependencies will be reduced by ~128KB.
While DOMParser and XMLSerializer are supported since Chrome 4, for environments that don't support DOMParser and/or XMLSerializer, there are polyfills like
xmldomandjsdom.xmldomis a lightweight (30KB) pure JS implementation of DOMParser and XMLSerializer with no dependencies on Node.js standard modules, so I use it in the updated tests to verify that our vast/vmap parsers will work well withxmldom.